-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change fractional (and others) to be a property, move quantizers #964
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmitrevs
commented
Feb 15, 2024
jmitrevs
commented
Feb 15, 2024
vloncar
added
please test
Trigger testing by creating local PR branch
and removed
please test
Trigger testing by creating local PR branch
labels
Feb 15, 2024
jmitrevs
added
please test
Trigger testing by creating local PR branch
and removed
please test
Trigger testing by creating local PR branch
labels
Feb 16, 2024
I think this looks good. Since I initially submitted this PR I can't approve it, but if you are happy with it, go ahead and merge it. |
vloncar
approved these changes
Feb 16, 2024
calad0i
pushed a commit
to calad0i/hls4ml
that referenced
this pull request
Feb 23, 2024
…tmachinelearning#964) * make fractional a property (plus similar modifications) * move quantizers to a new quantizers.py from types.py * remove explicit setting of fractional * remove setting for fractional that was missed * Clean up eq function and make everything a property * Add test for precision type creation * Nicer docstring for quantizers module * Move precision parsing test to test_types * Hardwire saturation bits of int type to 0 * Handle saturation bits for AC types correctly * Fix return value of __eq__ of PrecisionType base class --------- Co-authored-by: Vladimir Loncar <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR has two somewhat unrelated changes. I made the second after the first, so I don't have the second standalone (but can if necessary), and it's easy to extract just the first since it's the first commit. The changes are:
FixedPrecisionType
andIntegerPrecisionType
have the valueswidth
,integer
, andfractional
set as values stored in the classes. Though they are set properly initially, the values are mutable, so it's possible to edit the values after the fact. It's up to whoever modifies them to make sure they remain consistent. I think that's error-prone. For example, one could modifywidth
inIntegerPrecisionType
but leaveinteger
alone, leaving a malformed class. What this PR does is makefractional
a property ofFixedPrecisonType
, always returningwidth
-integer
, and a property ofIngegerPrecisionType
, always returning0
. Similarlyinteger
is a property ofIntegerPrecisionType
, just returningwidth
. They don't have setters. I also modified the saturation and rounding properties to return the default value if they are set to None, and added similar properties to IntegerPrecisionType but unchangeable, since they are not changeable in the type.types.py
into a newquantizers.py
file. For qonnx we will add additional quantizers, so my thinking is that it's better to have them in a separate file. But this is not a major issue, so I would be fine reverting it if there are objections.Type of change
Code restructuring
Tests
There should be no changes so the old tests should confirm that I didn't introduce an error.
Checklist
pre-commit
on the files I edited or added.